#219: dispose the Thread event from free_obj as well - #220
Merged
Conversation
php_error_cb() marks every live object as destructed before it bails out, so dtor_obj runs for none of them and teardown calls only free_obj. thread_object_dtor() was the sole caller of event->dispose(), and that dispose is the sole uv_close() of the cross-thread notify handle. After any fatal error the handle stayed open: uv_loop_close() returned EBUSY, and the loop's internals plus the persistent thread context leaked. free_obj now disposes the event too. Dispatching finally handlers stays in the dtor, since user code must not run on the fatal path.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #219.
php_error_cb()marks every live object as destructed before it bails out, sodtor_objruns for none of them and teardown calls onlyfree_obj.thread_object_dtor()was the sole caller ofevent->dispose(), and that dispose is the soleuv_close()of the cross-thread notify handle. After any fatal error the handle stayed open:uv_loop_close()returnedEBUSY, and the loop's internals plus the persistentzend_async_thread_context_tleaked. A debug build reported oneleftover libuv handleline per live thread.free_objnow disposes the event too, and releases the finally-handler array. Dispatching those handlers stays in the dtor, since user code must not run on the fatal path.Timers, sockets, signals and
ThreadPoolwere checked on the same fatal path and release their handles already; onlyThreadwas affected.Test
tests/thread/081-thread_fatal_disposes_notify_handle.phptruns four threads, keeps them reachable and exhausts the memory limit. A debug build names each surviving handle on stderr, which run-tests folds into the compared output, so the leak shows up as extra lines behind the fatal error — hence no trailing%Ain--EXPECTF--.Verified by reverting the fix: the test then fails on exactly the four
leftover libuv handlelines. Full local suite with the fix: 1228 tests, 0 failures.